fix(server): prevent unrelated sandbox deletes from blocking deletion events#2340
fix(server): prevent unrelated sandbox deletes from blocking deletion events#2340pimlock wants to merge 5 commits into
Conversation
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Label |
|
/ok to test 0f18d50 |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test 8283eec |
| self.cleanup_sandbox_owned_records(&sandbox).await; | ||
| self.sandbox_index.update_from_sandbox(&transition.deleting); | ||
| self.sandbox_watch_bus.notify(&candidate_id); | ||
| drop(guard); |
There was a problem hiding this comment.
Does returning before we call drop(guard) mean that we run the risk of keeping the lock? (Does Rust have an equivalent to a defer drop(guard)?
There was a problem hiding this comment.
In this case, the guard is dropped explicitly, so it's released before the function returns. Without that, it happens automatically when the value goes out of scope (e.g. function returns). They refer to it as RAII.
Btw using it after drop would fail the compilation, as drop takes the ownership.
I don't know how common this approach of dropping explicitly is, alternatively the part that needs the lock could be a separate function and then the explicit drop wouldn't be needed.
This was first pass the agent took (after iterating on the plan), so it may need some tweaking. One thing I'd like to have is that the sandbox delete lock cannot be acquired if the global lock is acquired, otherwise deadlocks could happen. I think there should be a way of representing it with the type system to make that enforced, but need to spend some more time on this.
| .ok_or_else(|| Status::not_found("sandbox not found"))?; | ||
| let candidate_id = candidate.object_id().to_string(); | ||
| let delete_gate = self.delete_gates.gate_for(&candidate_id); | ||
| let _delete_guard = delete_gate.lock().await; |
There was a problem hiding this comment.
This is dropped when the variable goes out of scope, so right before this function returns.
| let delete_gate = runtime.delete_gates.gate_for(original.object_id()); | ||
| let delete_guard = delete_gate.lock().await; |
There was a problem hiding this comment.
Under which condition is it required to take a lock? Some code (possibly only tests) call gate_for without a subsequent .lock().await.
There was a problem hiding this comment.
The lock ensures only one delete operation is happening at the same time for a given sandbox. Having multiple run at the same time introduces issues related to how the cleanup is performed and could cause inconsistent state in DB.
Before this change, we'd acquire the global lock for the whole deletion process, so the limitation was that only one sandbox operation (deletion or otherwise) could be happening at the same time. That included invoking driver.delete, which may be slow (waiting for the process to respond to sigterm/sigkill).
This applies the global lock more narrowly and uses local lock for the deletion process (the deletions of unrelated sandbox are independent).
This was the simplest solution we found with the agent, it's not bullet proof, of course it works only for a single gateway, if the mechanism relied on DB for leasing the deletion process, that would work if we have multiple gateways, but that seemed too complex for this fix. But it maybe worth exploring other options.
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
|
/ok to test 6a15fd9 |
PR Review StatusValidation: This PR is project-valid because it implements the maintainer-authored, agent-ready gateway/compute fix in #2326 and stays within that accepted deletion-concurrency scope. Head SHA: Review findings:
Thanks @elezar — I checked the lock-lifetime and acquisition-order concerns you raised. The current head's Docs: Checks: required Branch Checks, Helm Lint, and E2E are currently green, but review findings still require an author update. Next state: |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Review follow-upAddressed the pre-spawn identity-binding finding in The failed-flight behavior is intentionally serialized retry rather than result-sharing single-flight:
The new deterministic The architecture note now states that gates are process-local, serialize attempts rather than share results, and rely on persisted resource-version checks as the cross-replica safety boundary. |
Re-check After Author UpdateThanks @pimlock. I re-evaluated head What I checked: the public gRPC delete boundary, the pre-spawn identity binding, per-ID gate and global-lock acquisition, queued/canceled request behavior, failure recovery, watcher/reconciliation CAS fencing, the new deterministic tests, architecture docs, and the E2E polling helper. Resolved from the prior gator review:
Blocking findings:
Non-blocking follow-ups:
Docs: architecture coverage is appropriate; this remains an internal correctness fix, so Fern docs are not required. Checks: Branch Checks and Helm Lint are green for this head. Required E2E is still pending, but author changes are required before pipeline results can advance gator. Disposition: the prior findings are resolved as described above, but the two races remain blocking. Next state: |
Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>
Review follow-upAddressed all findings from the latest gator review in
The PR description and architecture note now document the cancellation commitment point. |
Re-check After Author UpdateThanks @pimlock. I re-evaluated head What I checked: the complete base-to-head diff, handler/compute identity flow, the delete gate and global-lock commitment point, driver deletion behavior, watcher removal and name reuse, recovery CAS handling, deterministic tests, architecture docs, and E2E polling. Resolved from the prior gator review:
Blocking finding:
Please make the stable ID authoritative throughout driver deletion. Kubernetes should verify the ID label and use an object-identity/UID precondition; container drivers should reject mismatches and remove an immutable ID-matching target; VM must not fall back to name when an ID was supplied. Add deterministic coverage where watcher removal and name reuse occur after the driver call begins but before it resolves its backend target. Non-blocking follow-ups:
Docs: no Fern update is required; this remains internal lifecycle correctness. The architecture doc is the right surface with the clarification above. Tests: Disposition: the prior findings are resolved, but the end-to-end stable-ID race remains blocking. Head SHA: |
Response to the stable-ID findingI agree with the desired end state: when The ID/name pairing predates this change. The normal single-gateway delete path in this PR cannot produce the retargeting sequence by itself:
For the stale The old global lock masked this limitation by preventing the gateway from applying watcher removal and freeing the name during any driver call. This PR intentionally allows watcher processing to continue so an unrelated sandbox event cannot be head-of-line blocked. Making the watcher acquire each sandbox delete gate would recreate that head-of-line blocking because the watcher stream is processed sequentially. Fixing only Given the independent-disappearance prerequisite and the accepted single-gateway scope, I propose treating this as the focused follow-up in #2347 rather than expanding #2340 into a cross-driver lifecycle-contract change. |
Summary
Prevent one slow sandbox deletion from blocking unrelated watcher events or deletes. The gateway now serializes duplicate deletes per stable sandbox ID, persists and recovers deletion transitions with version checks, and performs backend calls outside the gateway-wide state guard.
This global locking has been causing E2E tests to flake (see the issue for examples).
Overview of the issue and approach.
Before and after
Before
The delete request held the global synchronization lock while waiting for the compute driver. Watcher events and unrelated deletes could not acquire the same lock and therefore stalled behind the driver call.
flowchart TD deleteA["Delete sandbox A"] --> global["Acquire global lock"] global --> deleting["Persist phase: Deleting"] deleting --> driver["Call compute driver<br/>while holding global lock"] driver --> release["Release global lock"] watcher["Watcher event"] --> blocked["Wait for global lock"] deleteB["Delete sandbox B"] --> blocked release --> blockedAfter
The request resolves the sandbox's stable ID, then acquires that ID's gate and the global lock before it starts owned work. Cancellation while either lock is pending leaves no queued worker behind. Once both locks are held, spawning the worker is the commitment point: it persists the transition, releases the global lock, and calls the driver while holding only the per-ID gate. Delete attempts for the same ID never overlap; an active waiter may retry after a failed attempt restores a deletable state.
flowchart TD deleteA["Delete sandbox A"] --> idA["Resolve stable ID A"] idA --> gateA["Acquire delete gate A<br/>in request future"] gateA --> globalA["Acquire global lock<br/>in request future"] globalA --> commit["Spawn owned worker<br/>commitment point"] commit --> deletingA["CAS phase to Deleting"] deletingA --> releaseGlobal["Release global lock"] releaseGlobal --> driverA["Call compute driver<br/>while holding only gate A"] driverA --> recover["Reacquire global lock if cleanup<br/>or recovery is needed"] recover --> releaseGate["Release delete gate A"] canceled["Canceled duplicate A"] -. "drops while waiting;<br/>no worker exists" .-> gateA watcherAfter["Watcher event"] --> globalOther["Acquire global lock briefly"] deleteB["Delete sandbox B"] --> gateB["Acquire delete gate B"] gateB --> globalOther globalOther --> continue["Continue while driver A is running"]Related Issue
Closes #2326
Changes
SandboxDeleteGuardproving that the ID-scoped gate is held.The delete guard makes the intended lock order visible and harder to violate within the delete workflow, but it does not prevent a future caller from acquiring the global lock before requesting a delete gate. Enforcing that globally would require ranked-lock tracking or a broader refactor that restricts all direct lock acquisition, which is outside this PR's scope.
Deviations from Plan
Settings cleanup intentionally retains its existing best-effort name-based behavior. An earlier atomic persistence extension was removed to keep this PR scoped to the deletion concurrency issue.
Testing
mise run pre-commitpassesTests added or updated:
Checklist